Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix integer overflow in midi parser sample count calculation (bug #200) #220

Merged
merged 1 commit into from
Nov 5, 2020
Merged

fix integer overflow in midi parser sample count calculation (bug #200) #220

merged 1 commit into from
Nov 5, 2020

Conversation

sezero
Copy link
Contributor

@sezero sezero commented Nov 5, 2020

This fixes the 'impossibly long duration with crafted midi file' issue
as reported in bug #200. Midi file to test:
https://github.com/SegfaultMasters/covering360/raw/master/wildmidi/1_hang_main_00

Notes:

Comments? @chrisisonwildcode, what do you say?

Please review thoroughly. I'm willing to revise according to critisisms.

@sezero sezero changed the title fix integer overflow in midi parser smallest delta calculation (bug #200) fix integer overflow in midi parser sample count calculation (bug #200) Nov 5, 2020
@psi29a
Copy link
Member

psi29a commented Nov 5, 2020 •
edited
Loading

Thanks for taking this on @sezero
I'm wondering if we should start collecting these midi files for integration testing (and smoke test) so that we don't regress when changing things in the future.

And yes, ubsan is great. :)

@sezero
Copy link
Contributor Author

sezero commented Nov 5, 2020

Thanks for taking this on @sezero

You're welcome

I'm wondering if we should start collecting these midi files for integration testing (and smoke test) so that we don't regress when changing things in the future.

Attaching the relevant bad midi here, then
1_hang_main_00.zip

And yes, ubsan is great. :)

Yeah. (The clang version helped me though, the gcc version did not..)

Should I merge?

@psi29a
Copy link
Member

psi29a commented Nov 5, 2020

Yes, please merge.

This just covers up the problem though, there are very real issues though I think in our parser, like the HMI support and daggerfall files.

@sezero sezero merged commit ddd719d into master Nov 5, 2020
@sezero
Copy link
Contributor Author

sezero commented Nov 5, 2020

Merged.
Backported to wildmidi-0.3 too: 775894c

@sezero sezero deleted the bug-200 branch November 5, 2020 08:13
@sezero
Copy link
Contributor Author

sezero commented Nov 5, 2020

This just covers up the problem though, there are very real issues though I think in our parser, like the HMI support and daggerfall files.

Well, the daggerfall files (#176) I still don't know why 0.3 branch is good but 0.4 is trouble..

As for hmi and hmp: PING #181 :) Please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sezero @psi29a